Use parseInt instead of | operator for integer validation #147
Conversation
Seems like the | operator check will return a false negative given a sufficiently large integer
| @@ -5,7 +5,7 @@ import isAbsent from './util/isAbsent'; | |||
|
|
|||
| let isNaN = value => value != +value | |||
|
|
|||
| let isInteger = val => isAbsent(val) || val === (val | 0) | |||
| let isInteger = val => isAbsent(val) || val === parseInt(val, 10) | |||
There was a problem hiding this comment.
parseInt isn't the right thing, here we don't want to coerce the value into a number. The reason the original code doesn't work with large ints, btw, is that JS really only has 32bit ints and the bitwise operators first convert the float to an int then truncate it. This leads to overflows in the large number case.
I'm a bit torn, the original method is more "correct" even if it does seemingly odd things. If we were gonna change it tho, we should use the Number.isInteger logic e.g. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isInteger#Polyfill
There was a problem hiding this comment.
I think the coercion is fine if strict equality is being used (===)
There was a problem hiding this comment.
But any approach that makes false negatives go away is fine by me 👍
There was a problem hiding this comment.
+1 for @jozanza approach, parseInt + strict equality is everything that's needed, and it's more readable.
There was a problem hiding this comment.
I finally realized the flaw of the parseInt approach:
9999999999999999999.9 === parseInt(9999999999999999999.9, 10)
// trueNumbers can't be that large in JS, so it's basically 10000000000000000000 === 10000000000000000000 after a certain point.
I'll work on adding the Number.isInteger logic to this PR when I get a chance @jquense
This works correctly for large numbers. Related to jquense#147
This works correctly for large numbers. Related to jquense#147
This works correctly for large numbers. Related to jquense#147
This works correctly for large numbers. Related to jquense#147
|
parseInt('123456789a', 10) returns 123456789. Why not using type coercion: '123456789a' *1 . It is also 4 times faster. |
BREAKING CHANGE: use Number.isInteger. This works correctly for large numbers. Related to #147
Seems like the
|operator check will return a false negative given a sufficiently large integer. This PR solves #50